Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

fix(import): support for nested namespace packages #1805

Merged
merged 2 commits into from
Oct 9, 2021
Merged

fix(import): support for nested namespace packages #1805

merged 2 commits into from
Oct 9, 2021

Conversation

kirat-singh
Copy link
Contributor

If multiple directories in sys.path provide a nested namespace package, then jedi would only visit the first directory which
contained the package. Fix this by saving the remaining path list in the ImplicitNamespaceValue and add a test for it.

Added test namespace packages to demonstrate the failure:

namespace1/ namespace1/pkg1 namespace1/pkg1/pkg2 namespace1/pkg1/pkg2/mod1.py namespace2 namespace2/pkg1 namespace2/pkg1/pkg2 namespace2/pkg1/pkg2/mod2.py

If both namespace1 and namespace2 are added to sys path, then jedi sees only mod1 without the fix. After the fix it correctly sees both mod1 and mod2.

@kirat-singh
fix(import): support for nested namespace packages
53e8370 
If multiple directories in sys.path provide a nested namespace
package, then jedi would only visit the first directory which
contained the package.  Fix this by saving the remaining path list in
the ImplicitNamespaceValue and add a test for it.
@kirat-singh
Copy link
Contributor Author

Whoops this is a duplicate of #1784 I'm happy to withdraw this if the other one is approved.

@codecov-commenter
Copy link

Codecov Report

Merging #1805 (53e8370) into master (65bc1c1) will not change coverage.
The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #1805   +/-   ##
=======================================
  Coverage   94.50%   94.50%           
=======================================
  Files          80       80           
  Lines       11817    11817           
=======================================
  Hits        11168    11168           
  Misses        649      649
Impacted Files Coverage Δ
jedi/inference/imports.py 98.62% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 65bc1c1...53e8370. Read the comment docs.

@davidhalter
Copy link
Owner

I guess the isinstance there is kind of weird and does not make sense. I guess just change it to

        file_io_or_ns, is_pkg = inference_state.compiled_subprocess.get_module_info(
            string=import_names[-1],
            path=paths,
            full_name=module_name,
            is_global_search=False,
        )
        if is_pkg is None:
            return NO_VALUES

The for loop here is removed and not needed anymore. I do not know why it was there in the first place. Probably some refactorings that made it unnecessary.

@davidhalter
Copy link
Owner

#1784 is a duplicate, but it changes some other things as well, so we might as well merge both and resolve the merge conflict :)

The earlier fix will probably just win ;-).

@kirat-singh
Copy link
Contributor Author

thanks for looking, and agree should have simplified. changed it as you suggested.

@davidhalter davidhalter merged commit bb5bed4 into davidhalter:master Oct 9, 2021
@davidhalter
Copy link
Owner

Thanks!

@davidhalter davidhalter mentioned this pull request Nov 16, 2021
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@kirat-singh @codecov-commenter @davidhalter